-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
uses clap instead of custom cli arg parsing #271
base: master
Are you sure you want to change the base?
Conversation
Oh, that's great, thank you! 👍 I'll review as soon as I have time to work on gnirehtet. 😉 |
This commit uses [clap](https://github.com/clap-rs/clap) to handle command line arugments instead of a custom rolled solution. The benefit that `clap` handles all validation as well as providing many "quality of life" or UX improvements such as hidden alias, suggestions on typo, auto-generated help, etc. A previous branch of this project used [structopt](https://github.com/TeXitoi/structopt) (which internally uses `clap`). `structopt` makes heavy use of Rust's Custom Derive feature to make the code less verbose and more ergonomic. `clap` is nearing it's v3 release, which pulls `structopt` into the mainline code base. This commit does not use the v3 `structopt`-like feature-set since that release is still in alpha. This meanas the code is more verbose (yet just as performant), and could be changed to the `structopt`-like features once `clap` v3 is officially released. This commit also does not utilize some of `clap`'s other features like the ability to generate [Shell Completion scripts](https://docs.rs/clap/2.33.0/clap/struct.App.html#method.gen_completions) which could be added later if desired. This commit is *nearly* a 1:1 translation from the original home rolled, to using `clap`. I say "nearly" because `clap` adds many features on top of what the home rolled solution provided "for free" as in with no additional configuration. Those features include: * Argument validation: `--routes`, `--dns-servers`, and `--port` all now have built in validation. If someone enters an IP that isn't valid, or a port that isn't valid an error is returned with a friendly message prior to advancing into `gnirehtet` code. `--routes` also checks the CIDR for validity as well. * Auto generated help message * Suggestions on typos (this feature could be turned off to reduce the binary size, see below) * [Hidden Command aliases](https://docs.rs/clap/2.33.0/clap/struct.App.html#method.alias); The following hidden aliases have been added for commands: * `rt` which maps to `run`: I noticed `gnirehtet rt` used to be used for `gnirehtet run` * Long options for arguments (i.e. `--port` as well as `-p`) * [Hidden Argument aliases](https://docs.rs/clap/2.33.0/clap/struct.Arg.html#method.alias); the following hidden aliases have been added for arguments (which provides help for common typos): * `--dns-server` -> `--dns` -> `--dns-servers`: All of these map to the same thing * `--route` -> `--routes` * Long and Short help messages: `clap` allows one to specify a short message for arguments and commands when `-h` is used, and display a longer more comprehensive message when `--help` is used. * Multiple ways to get help: `clap` provides `-h`|`--help` for the base `gnirehtet` command, as well as all it's subcommands, i.e. `gnirehtet run --help`. There is also a `help` subcommand `gnirehtet help` which can be used alone or to display other commands, i.e. `gnirehtet help run`. This helps because people assume various forms of help, and should be able to find the help messages no matter how they originally try it. Binary Size: I see that binary size is a concern. This is the size after this commit: ``` ❯ ls -l target/release/gnirehtet .rwxrwxr-x 2.2M kevin 23 Feb 13:22 target/release/gnirehtet ❯ strip -g target/release/gnirehtet ❯ ls -l target/release/gnirehtet .rwxrwxr-x 1.1M kevin 23 Feb 13:22 target/release/gnirehtet ❯ strip -s target/release/gnirehtet ❯ ls -l target/release/gnirehtet .rwxrwxr-x 973k kevin 23 Feb 13:22 target/release/gnirehtet ``` So it does increase the binary size, but not dramatically. By further turning off [`clap`'s default cargo features](https://github.com/clap-rs/clap/#optional-dependencies--features), the size may be able to be reduced even more. However, I also am a firm believer that these slight increases are well worth the features they provide. Relates to Genymobile#243 wip: clap
c72dcd6
to
d0d4479
Compare
Hi, Sorry for the delay. I quickly looked at it today, but I have this issue:
This is weird, because "port" is not passed as argument to "uninstall" in clap. Btw, I rebased on |
serial: m.value_of("serial").map(ToOwned::to_owned), | ||
dns_servers: m.value_of("dns-servers").map(ToOwned::to_owned), | ||
routes: m.value_of("routes").map(ToOwned::to_owned), | ||
port: value_t_or_exit!(m.value_of("port"), u16), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this should not be written in the current way, as some sub-commands does not even have port
defined, even though port
has a default value?
This commit uses clap to handle
command line arugments instead of a custom rolled solution. The benefit
that
clap
handles all validation as well as providing many "quality oflife" or UX improvements such as hidden alias, suggestions on typo,
auto-generated help, etc.
A previous branch of this project used
structopt (which internally uses
clap
).structopt
makes heavy use of Rust's Custom Derive feature tomake the code less verbose and more ergonomic.
clap
is nearing it's v3release, which pulls
structopt
into the mainline code base. Thiscommit does not use the v3
structopt
-like feature-set since thatrelease is still in alpha. This meanas the code is more verbose (yet
just as performant), and could be changed to the
structopt
-likefeatures once
clap
v3 is officially released.This commit also does not utilize some of
clap
's other features like theability to generate Shell Completion scripts
which could be added later if desired.
This commit is nearly a 1:1 translation from the original home rolled,
to using
clap
. I say "nearly" becauseclap
adds many features on topof what the home rolled solution provided "for free" as in with no
additional configuration. Those features include:
--routes
,--dns-servers
, and--port
all nowhave built in validation. If someone enters an IP that isn't valid, or a
port that isn't valid an error is returned with a friendly message prior
to advancing into
gnirehtet
code.--routes
also checks the CIDR forvalidity as well.
binary size, see below)
commands:
rt
which maps torun
: I noticedgnirehtet rt
used to be used forgnirehtet run
--port
as well as-p
)for arguments (which provides help for common typos):
--dns-server
->--dns
->--dns-servers
: All of these map to the same thing--route
->--routes
clap
allows one to specify a shortmessage for arguments and commands when
-h
is used, and display alonger more comprehensive message when
--help
is used.clap
provides-h
|--help
for the basegnirehtet
command, as well as all it's subcommands, i.e.gnirehtet run --help
. There is also ahelp
subcommandgnirehtet help
whichcan be used alone or to display other commands, i.e.
gnirehtet help run
. This helps because people assume various forms of help, and shouldbe able to find the help messages no matter how they originally try it.
Binary Size
I see that binary size is a concern. This is the size after this commit:
So it does increase the binary size, but not dramatically. By further
turning off
clap
's default cargofeatures,
the size may be able to be reduced even more. However, I also am a firm believer that these slight
increases are well worth the features they provide.
Relates to #243